Soft-deprecate the use of implicit reliance on NULL-terminated strings. #99806
Conversation
Ivorforce
force-pushed
the
string-deprecate-null-term
branch
15 times, most recently
from
1b8e3da
to
1cb5c1e
Compare
Ivorforce
force-pushed
the
string-deprecate-null-term
branch
from
1cb5c1e
to
9bc994c
Compare
|
Sounds interesting! Can you share the results of your profiling so we have an idea of what kind of performance differential to expect? |
Will do once I get everything rolling again! It's still stuck between not compiling or causing segfaults, hopefully to resolve soon :) |
Ivorforce
force-pushed
the
string-deprecate-null-term
branch
12 times, most recently
from
691af48
to
54d429b
Compare
|
Hello @Ivorforce ! I hope you're doing well. The release team found out that you were updating your PR a lot lately. Could you add a tag in your commit message when CI checks are not needed? (see https://docs.github.com/en/actions/managing-workflow-runs-and-deployments/managing-workflow-runs/skipping-workflow-runs) It's because each time you push an update, even a tiny one, it triggers full CI checks. 😅 |
|
Yep, sorry, will do! |
Ivorforce
force-pushed
the
string-deprecate-null-term
branch
2 times, most recently
from
8913fbd
to
da45a5d
Compare
Ivorforce
force-pushed
the
string-deprecate-null-term
branch
from
da45a5d
to
af0de09
Compare
|
Tests are finally passing (locally anyway)! I managed to salvage this PR by doing less at once. The core is still the same - implicit conversions from The All in all, this PR should slightly speed up conversion from string literal to I recommend merging #99816 and #99817 before this one because this change includes the former ones, making the changelog more granular. Edit: One test is failing, I think that's just caches though? |
…icitly parse the given string to find its end. Add known-length constructors and explicit from_c_str static methods.
Ivorforce
force-pushed
the
string-deprecate-null-term
branch
from
af0de09
to
be06698
Compare
|
Holy shit, you have no idea how cathartic it is to see someone else take an interest in this topic. I'm 100% onboard with modernizing our strings to not rely on null-termination, especially considering |
|
I made a synthetic performance benchmark: https://onlinegdb.com/rzGUA045Ry This PR is not about performance, but I wanted to check anyway, since some string creators already automatically select the static length constructor with this PR. The speed gain is not very consistent (it's more consistent offline); unscientifically i would estimate it's about 9% for short length strings, 5% for medium length strings, and 4% for long length strings such as from canvas.glsl.gen.h. |
|
I'll bump the build, but I don't think it's a cache thing. |
"MVP" of addressing godotengine/godot-proposals#11249 and godotengine/godot-proposals#11258. The motivation for this direction are discussed there (and linked issues). This PR does not yet change assumptions about user-supplied strings;
NULLtermination checks are only avoided for static strings supplied from the codebase.This change is toggled with the c++-define
GODOT_SHOW_STR_DEPRECATIONS(off by default). For unowned string references,StrRangecan be used, which avoids allocation overhead. This is already employed insprintfandoperator+in this PR, because there are a lot of uses of these functions.This change is as minimal as I could realistically get it to be:
ustringhas quite a lot of changes, but most other files work exactly as before. This changes whenGODOT_SHOW_STR_DEPRECATIONSis toggled: Suddenly, the console explodes into a flurry of warnings, becausestrlenis used in a place where it may be inappropriate and wastes CPU cycles.This PR should already slightly improves performance by letting callers pass their
string[]asstring[]with known lengths, rather than having it recomputed immediately. Every change we will make to get rid of anotherGODOT_SHOW_STR_DEPRECATIONSdeprecation warning will improve the engine speed further. Every function we can make refactor to useStrRangeinstead of String will reduce allocations and improve engine speed.Please, participate in lively discussion! I realize merging this is kind of a large ask, especially from a pretty fresh contributor like myself. I tried to make the change as unopinionated as possible, and focused on just improving speed and conditions for future PRs to improve speed further.
Still, I personally think this direction is unavoidable sooner or later, if we want to make the best use of clock cycles for the hard parts elsewhere. Also, i have heard it mentioned a few times that the Godot source code has a side-mission as a learning tool - making computation explicit to avoid accidental promotions and data copies is very important in clean and fast code :)